Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

scanner: correct the autoProvision mechanism #147

Merged
merged 1 commit into from
Sep 27, 2024
Merged

scanner: correct the autoProvision mechanism #147

merged 1 commit into from
Sep 27, 2024

Conversation

Vicente-Cheng
Copy link
Collaborator 

- we should not set both `Spec.Filesystem.Provisioned` and
  `Spec.Provision` or the remove operation would be blocked if
  we only change one of these to false.

This is a side-effect that comes from 1877a4ef1743e4bb01b81179aacfe5b32da059ed.
For now, we will remove one of these two fields to ensure the delete operation
work. We still need mutator to help convert these two fields.

Problem:
The removal operation would be blocked if we only changed one of these to false.

Solution:
For now, we will remove one of these two fields to ensure the delete operation work.

Related Issue:

Test plan:

@Vicente-Cheng
Copy link
Collaborator Author

The CI will fail because the autoProvisioned behavior will be called on the current master code.

@tserong
Copy link
Contributor

tserong commented Sep 27, 2024

Shouldn't we remove bd.Spec.FileSystem.Provisioned = true and keep bd.Spec.Provision instead?

Also, there's another instance of this where we set both to true in updateDeviceStatus() in pkg/controller/blockdevice/controller.go

@Vicente-Cheng
scanner: correct the autoProvision mechanism
71820cc 
    - we should not set both `Spec.Filesystem.Provisioned` and
      `Spec.Provision` or the remove operation would be blocked if
      we only change one of these to false.

    This is a side-effect that comes from 1877a4e.
    For now, we will remove one of these two fields to ensure the delete operation
    work. We still need mutator to help convert these two fields.

Signed-off-by: Vicente Cheng <vicente.cheng@suse.com>
@Vicente-Cheng
Copy link
Collaborator Author

Shouldn't we remove bd.Spec.FileSystem.Provisioned = true and keep bd.Spec.Provision instead?

Both are ok to me.
I would like to keep bd.Spec.FileSystem.Provisioned = true because I want to align with the test case.
The current integration test still targets this field.

I am working on the mutator. After that, we can move the whole Spec.FileSystem.Provisioned to Spec.Provision
WDYT?

Also, there's another instance of this where we set both to true in updateDeviceStatus() in pkg/controller/blockdevice/controller.go

Nice catch, updated!

@tserong
Copy link
Contributor

tserong commented Sep 27, 2024

I am working on the mutator. After that, we can move the whole Spec.FileSystem.Provisioned to Spec.Provision
WDYT?

Cool, makes sense.

@Vicente-Cheng
Copy link
Collaborator Author

@Mergifyio backport v0.7.x

@mergify Mergify
Copy link

mergify bot commented Sep 27, 2024 •
edited
Loading

backport v0.7.x

✅ Backports have been created

@Vicente-Cheng Vicente-Cheng merged commit d5b7347 into harvester:master Sep 27, 2024
4 of 6 checks passed
@mergify mergify bot mentioned this pull request Sep 27, 2024
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bk201 bk201 bk201 approved these changes

@tserong tserong tserong approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@Vicente-Cheng @tserong @bk201